Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add separate min gas price option for txes with high gas wanted #777

Merged

Conversation

hleb-albau
Copy link
Contributor

@hleb-albau hleb-albau commented Jan 22, 2022

Closes: #724

Description

  • Refactor osmosis mempool filter
  • Add separate min gas price option for txes with high gas wanted

Open questions to reviewer:

I think we should not move HighGasTxThreshold parameter into consensus, but leave it just as a node option. If so, it's better to add it to app.toml

Other

Do not waste time for choosing words, if something is wrong just said it straightforward, i am OK with that.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah awesome job on this!

This is extremely well done imo, thanks for refactoring everything into Mempool options, and cleaning up the config parsing

x/txfees/types/options.go Outdated Show resolved Hide resolved
@ValarDragon
Copy link
Member

I think your right re: Not moving high gas thresholds as something done in consensus as well, and just keeping it in app.go. (Which this PR does)

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

Codecov Report

Merging #777 (24f9976) into main (2e3966f) will increase coverage by 0.01%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #777      +/-   ##
==========================================
+ Coverage   19.96%   19.97%   +0.01%     
==========================================
  Files         189      189              
  Lines       24542    24547       +5     
==========================================
+ Hits         4899     4904       +5     
  Misses      18788    18788              
  Partials      855      855              
Impacted Files Coverage Δ
x/txfees/keeper/feedecorator.go 75.00% <90.00%> (+2.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e3966f...24f9976. Read the comment docs.

@hleb-albau hleb-albau force-pushed the 724_high_gas_txes_fee_options branch from 32cac99 to 24f9976 Compare January 25, 2022 00:39
@ValarDragon ValarDragon merged commit f0842b9 into osmosis-labs:main Jan 25, 2022
@hleb-albau hleb-albau deleted the 724_high_gas_txes_fee_options branch January 25, 2022 09:23
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

x/txfees: Make an ante-handler filter for recognizing High gas txs, and having a min gas price for them
4 participants